Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update a few function definitions #66315

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Update a few function definitions #66315

merged 1 commit into from
Oct 22, 2024

Conversation

tyxla
Copy link
Member

@tyxla tyxla commented Oct 22, 2024

What?

Changing a few arrow functions to be regular functions.

Why?

To unblock #65463.

For some reason @typescript-eslint/typescript-estree breaks parsing those constructs when an arrow function is used, while using a regular function works well.

When fixing those, we're able to run react-scanner properly without it failing on those functions

How?

We're just changing a few arrow functions to be proper functions, the rest is the same

Testing Instructions

  • All checks should be green. This is a minor syntax change.
  • If you want to test with React Scanner:
    • Checkout Introduce React Scanner for component usage stats #65463
    • Run npm install
    • Run npm run component-usage-stats > stats.json
    • You'll see 3 errors (see "screenshots or screencast" below)
    • Run git cherry-pick 1257ccef14c3e0165a71a1195237ec37724f0273 to get the commit from this branch.
    • Run npm run component-usage-stats > stats.json
    • You'll see no errors.

Testing Instructions for Keyboard

Same

Screenshots or screencast

Errors we see with #65463:

Failed to parse: gutenberg/packages/components/src/spacer/hook.ts
Failed to parse: gutenberg/packages/data-controls/src/index.ts
Failed to parse: gutenberg/packages/fields/src/mutation/index.ts

@tyxla tyxla added the [Type] Code Quality Issues or PRs that relate to code quality label Oct 22, 2024
@tyxla tyxla requested a review from a team October 22, 2024 10:47
@tyxla tyxla self-assigned this Oct 22, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 22, 2024 •
edited
Loading

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@tyxla tyxla mentioned this pull request Oct 22, 2024
Merged
ciampo
ciampo approved these changes Oct 22, 2024
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

It's werid that those function definitions caused problems in the first place. Maybe we should open an issue with the @typescript-eslint/typescript-estree project

@tyxla
Copy link
Member Author

tyxla commented Oct 22, 2024

BTW the error comes from #66213 and I'm resolving it in #66316

@tyxla
Copy link
Member Author

tyxla commented Oct 22, 2024

Maybe we should open an issue with the @typescript-eslint/typescript-estree project

Will do 👍

@tyxla
Copy link
Member Author

tyxla commented Oct 22, 2024

Maybe we should open an issue with the @typescript-eslint/typescript-estree project

Will do 👍

Looks like it's been a known issue for a while: typescript-eslint/typescript-eslint#1236

@github-actions GitHub Actions
Copy link

Flaky tests detected in 1257cce.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11458582363
📝 Reported issues:

@tyxla tyxla force-pushed the update/some-function-definitions branch from 1257cce to 6980ce8 Compare October 22, 2024 11:38
@tyxla tyxla enabled auto-merge (squash) October 22, 2024 11:40
@tyxla tyxla merged commit 9fcb43b into trunk Oct 22, 2024
62 of 63 checks passed
@tyxla tyxla deleted the update/some-function-definitions branch October 22, 2024 12:13
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 22, 2024
@cbravobernal cbravobernal added the [Package] DataViews /packages/dataviews label Oct 30, 2024
karthick-murugan pushed a commit to karthick-murugan/gutenberg that referenced this pull request Nov 13, 2024
@tyxla @ciampo @karthick-murugan
Update a few function definitions (WordPress#66315)
3775272 
Co-authored-by: tyxla <[email protected]>
Co-authored-by: ciampo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciampo ciampo ciampo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

@nerrad nerrad Awaiting requested review from nerrad nerrad is a code owner

Assignees

@tyxla tyxla

Labels
[Package] DataViews /packages/dataviews [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 19.6
Development

Successfully merging this pull request may close these issues.
3 participants
@tyxla @ciampo @cbravobernal